Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

QuickEdit: Add Featured Image Control #64496

Merged
merged 51 commits into from
Oct 23, 2024

Conversation

gigitux
Copy link
Contributor

@gigitux gigitux commented Aug 14, 2024

What?

Part of #64519

This PR adds the Featured Image Control for the Page view.

Screen.Capture.on.2024-09-18.at.18-09-37.mov

View mode

Grid Table Layout
image image image

Edit

Without Image With Image
image image

Additional Changes

Render component and view prop

With this PR, the render component will now receive the view prop. This addition is necessary because the component’s layout needs to adapt based on the view. Similar cases include the Title field, where different layouts are required depending on the view.

[ LAYOUT_TABLE, LAYOUT_GRID ].includes( viewType ) &&
item.status !== 'trash';

Fix error on bulk edit mode

Screen.Capture.on.2024-09-18.at.18-01-00.mp4
Uncaught TypeError: Cannot read properties of null (reading 'status')

Currently, there is an error when the user edits a field in bulk mode. This PR fixes this error.

Attachment type

This PR updates the Attachment type.

Why?

How?

Testing Instructions

Ensure that Quick Edit in DataViews and Quick Edit in DataViews are enabled.

  1. Visit the Site Editor
  2. Click on pages.
  3. Toggle to the table view.
  4. Click on the settings.
  5. Make the feature image field visible.
  6. Click on details panel icon.
  7. Ensure that the Featured Image option is available.
  8. Click on it.
  9. Ensure that the Media Gallery opens and click on an image.
  10. Save.
  11. Ensure that the image is set as the featured image to the post.
  12. Toggle to the grid view.
  13. Ensure that the image is rendered correctly.
  14. Toggle to the list view.
  15. Ensure that the image is rendered correctly.

Testing Instructions for Keyboard

Screenshots or screencast

@youknowriad youknowriad added [Type] Experimental Experimental feature or API. [Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond labels Aug 14, 2024
@jameskoster
Copy link
Contributor

To be clear, I think the Featured Image control should remain as it is currently in terms of design and position.

However I also recognise that panel Data Forms should be able to handle image fields, respecting the established conventions of this layout. Here's a mockup and flow that could achieve that:

image

What do you think?

@gigitux
Copy link
Contributor Author

gigitux commented Aug 16, 2024

To be clear, I think the Featured Image control should remain as it is currently in terms of design and position.

However I also recognise that panel Data Forms should be able to handle image fields, respecting the established conventions of this layout. Here's a mockup and flow that could achieve that:

image What do you think?

Thanks for your feedback! It makes sense! I'm going to implement your feedback! 🚀

@gigitux gigitux force-pushed the add/page-image-control branch 2 times, most recently from 2133ef7 to 0a2c977 Compare August 22, 2024 13:44
@gigitux
Copy link
Contributor Author

gigitux commented Aug 22, 2024

Hey @jameskoster, I've implemented your suggestion:

Screen.Capture.on.2024-08-22.at.15-42-51.mp4

Let me know if, from the design perspective, there is something to address.

I’m not marking this PR as ready for review because we still need to implement the default edit control for the image field. Since DataViews is a general-purpose library, we shouldn’t rely on the WordPress Media Gallery for the generic control. Instead, I suggest using the Operating System’s file selection option:

Screen.Capture.on.2024-08-22.at.15-52-20.mp4

However, please note that in this case, we won’t have access to the filename once the file is uploaded.

@jameskoster
Copy link
Contributor

Thanks for the update. The flow seems pretty good, but there are some design details to fix.

Screenshot 2024-08-22 at 16 06 34
  • Border radius is missing
  • Border should use $border-width (not the focus width)
  • Border color should be $gray-300
  • Hover background should be $gray-100
  • Thumbnail should be 24px
  • The thumbnail appearance should match unset color swatches
  • Gap between swatch and text should be 8px (seems you can remove the margin on the span)
  • Padding should be 8px 12px

Also here:

Screenshot 2024-08-22 at 16 09 13
  • Thumbnail should be 24px with $radius-small border-radius. Ideally there should be a semi-transparent black outline so that white images are still visible. You can see an example in data views if you toggle on the preview.
  • Image name container height should be 24px to align with thumbnail
  • Filename should be 12px, with 16px line-height
  • Gap between thumbnail and text should be 8px (again, removing the margin on the span should fix)
  • Padding should be 8px 12px
  • Reset button size should be small (24px) to align with the image name / thumbnail
  • Ideally the filename would be truncated so that you can see the filetype. Perhaps this can be accomplished by using the Text component with the truncate and ellipsizeMode props.

Here's a mockup that hopefully clarifies some of the details:

Screenshot 2024-08-22 at 16 20 23

@gigitux
Copy link
Contributor Author

gigitux commented Aug 23, 2024

With 9a39fe6, I addressed your feedback!

Screen.Capture.on.2024-08-23.at.10-14-17.mp4

The only remaining challenge is that the filename can only be truncated to a fixed number of characters, which may result in it not always ending directly under the remove button.

Comment on lines 485 to 490
event.target.tagName.toLowerCase();
// Prevent opening the media modal when clicking on the button/icon.
if (
element !== 'button' &&
element !== 'svg'
) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is that there's a button within a clickable area, and clicking the button triggers a separate action. I'm not sure if this is the best way to handle it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we should avoid nested buttons. We had a similar issue in data views list layout... this UI:

Screenshot 2024-08-23 at 11 41 02

Perhaps some of the techniques there can be borrowed?

render={ ( { open } ) => {
return (
<div
role="button"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jameskoster
Copy link
Contributor

Nice, it's getting there :)

Border-radius on the thumbnail and button are still missing.

Truncation of the file name seems to occur slightly before it's necessary. In the screenshot below note that there's more space the title could occypy.

Screenshot 2024-08-23 at 11 45 55

I know the button that appears in the panel is a work in progress, but I wonder if it could have the same appearance as the author field. What do you think?

Screenshot 2024-08-23 at 11 49 09

@gigitux
Copy link
Contributor Author

gigitux commented Aug 23, 2024

Truncation of the file name seems to occur slightly before it's necessary. In the screenshot below note that there's more space the title could occypy.

As I mentioned in the previous comment, this could be complex to achieve given that Text components can only truncate to a fixed number of characters, which may result in it not always ending directly under the remove button.

I know the button that appears in the panel is a work in progress, but I wonder if it could have the same appearance as the author field. What do you think?

Do you mean something like this?

image

@jameskoster
Copy link
Contributor

given that Text components can only truncate to a fixed number of characters

Are you sure? It seems to truncate based on width when truncate is true and numberOfLines is 1?

Do you mean something like this?

Sorry I should have been more explicit. See the mockup below and notice that the Author and Featured Image buttons (outlined in red) share the same characteristics; thumbnail size, gap, spacing, etc.

Screenshot 2024-08-23 at 12 23 12

@gigitux
Copy link
Contributor Author

gigitux commented Aug 23, 2024

Are you sure? It seems to truncate based on width when truncate is true and numberOfLines is 1?

The documentation says that when ellipsizeMode is middle (our case), the limit property is required:

image

´Limit` is the max number of characters to be displayed:
image

So, if we want to truncate in the middle (so, showing the extension), we need to explicit the max number of characters.

Sorry I should have been more explicit. See the mockup below and notice that the Author and Featured Image buttons (outlined in red) share the same characteristics; thumbnail size, gap, spacing, etc.

No worries! Gotcha! Thanks! 🙇

),
enableSorting: false,
},
featuredImageField,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that we're moving the dataviews fields to the fields package one by one 👍 and fixing them (removing dependencies...)

@@ -34,10 +34,10 @@ export function getItemTitle( item: Post ) {
if ( typeof item.title === 'string' ) {
return decodeEntities( item.title );
}
if ( 'rendered' in item.title ) {
if ( item.title && 'rendered' in item.title ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In which case do we call this function with a non existent item.title ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember encountering some errors in the past, which led me to add this check. However, I’m no longer able to replicate the issue. I’m happy to revert the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted the changes with b039310.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work here.

I know the constraint of "no layout" in render is creating a non ideal result in the list view but hopefully we can address later with a better solution.

@youknowriad
Copy link
Contributor

(looks like you need to run.npm install and commit the lock)

@youknowriad
Copy link
Contributor

The main downside of this approach is that the click event is no longer configured for the grid view.

I think we should follow-up by having a special callback selectItem similar to getItemId in dataViews, that layouts can trigger when they want: "grid view" can trigger it when clicking the media and primary field, table view can trigger it when clicking primary fields...

@gigitux
Copy link
Contributor Author

gigitux commented Oct 22, 2024

(looks like you need to run.npm install and commit the lock)

Fixed with 42f90f4.

@youknowriad
Copy link
Contributor

I think we should follow-up by having a special callback selectItem similar to getItemId in dataViews, that layouts can trigger when they want: "grid view" can trigger it when clicking the media and primary field, table view can trigger it when clicking primary fields...

@gigitux Do you think this one is something you can help with? It feels like the most important follow-up here.

@gigitux
Copy link
Contributor Author

gigitux commented Oct 22, 2024

Thanks for your review! Based on our discussion, I created three issues:

@gigitux Do you think this one is something you can help with? It feels like the most important follow-up here.

Yes, I will work on #66338!

@youknowriad youknowriad enabled auto-merge (squash) October 23, 2024 13:32
@youknowriad youknowriad merged commit 77f62e0 into WordPress:trunk Oct 23, 2024
63 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.6 milestone Oct 23, 2024
}
}

.dataforms-layouts-panel__field-control {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do absorb this for any field of type media so consumers don't have to use dataviews classes (they're not public API). Would it be resolved if we add a dataviews type media that absorbs these styles and the "featured media field" uses?


export const FeaturedImageView = ( {
item,
}: DataViewRenderFieldProps< BasePost > ) => {
Copy link
Member

@oandregal oandregal Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems this could be implemented in a way that is not specific to "featured media", but rather to a media type. For example, what if we do something along these lines:

FeaturedImageView = ({
  item,
}) {
  const url = item.getValue();
  return (<img src={ url } );
}

If this was a media type field, the field "featured media" could be declared as:

{
  id: 'featured-media',
  type: 'media',
  getValue: () => { /* convert item to URL here */ },
}

This means that field types would also provide a render default. I think we should assume field types should provide default for every field property.

@oandregal
Copy link
Member

What are the next steps to consolidate these two experiences for media?

Editor Quick Edit
Screenshot 2024-10-23 at 18 16 36 Screenshot 2024-10-23 at 18 16 46

@youknowriad
Copy link
Contributor

@oandregal yeah, my main idea there (not sure if it's tracked) is to remove the "layout" type in the "form" config and instead have a "form.fields" config that is like that:

[ 
   { field: 'featured-image', label: 'hidden', control: 'inline' }, { field: 'slug', label: 'side', control: 'dropdown' } 
]

or something like that.

@gigitux
Copy link
Contributor Author

gigitux commented Oct 24, 2024

@oandregal yeah, my main idea there (not sure if it's tracked) is to remove the "layout" type in the "form" config and instead have a "form.fields" config that is like that:

[ 
   { field: 'featured-image', label: 'hidden', control: 'inline' }, { field: 'slug', label: 'side', control: 'dropdown' } 
]

or something like that.

This is something that we are planning to work on it soon... cc @louwie17

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] DataViews Work surrounding upgrading and evolving views in the site editor and beyond [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants